vitest.config.ts (1)

1-24: Ensure __dirname works with your Vitest module format

If your project is using ESM (e.g. "type": "module" in package.json or TS compiled as ESM), __dirname won’t be defined in vitest.config.ts, which would break test startup. In that case, consider computing it from import.meta.url:

-import { defineConfig } from 'vitest/config';
-import { resolve } from 'node:path';
-
-export default defineConfig({
+import { defineConfig } from 'vitest/config';
+import { resolve, dirname } from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+const __dirname = dirname(fileURLToPath(import.meta.url));
+
+export default defineConfig({
   test: {
@@
   resolve: {
     alias: {
       '@spatialdata/core': resolve(__dirname, 'packages/core/src'),
       '@spatialdata/zarrextra': resolve(__dirname, 'packages/zarrextra/src'),
     },
   },
 });

If the repo is still CJS-only, the existing __dirname usage is fine, but it’s worth confirming the module type to avoid subtle config failures.

packages/core/src/store/index.ts (1)

82-119: Consider a safer toJSON fallback than returning this

toJSON() currently returns the instance itself when this.rootStore.tree is falsy, which can be surprising and risks odd serialization behavior (or recursion) if JSON.stringify is used on SpatialData. Since ConsolidatedStore.tree should normally exist, a minimal or empty fallback is safer:

   toJSON() {
-    if (!this.rootStore.tree) return this;
-    return serializeZarrTree(this.rootStore.tree);
+    if (!this.rootStore.tree) {
+      // Fallback to a minimal representation instead of the full instance
+      return { url: this.url };
+    }
+    return serializeZarrTree(this.rootStore.tree);
   }

The new toString() with a try/catch looks good and should make debugging bad metadata less painful.

packages/zarrextra/src/types.ts (1)

45-135: Align comments/types with v3-normalized metadata and consider widening fill_value

You now normalize v2 .zmetadata to the v3-shaped ZarrV3Metadata and type IntermediateConsolidatedStore as Store & { zmetadata: ZarrV3Metadata }, which is a clear internal contract; the surrounding comments about a v2/v3 union and ConsolidatedMetadata/MetadataFormat could be updated or trimmed so future readers don’t expect both shapes to appear at runtime.

Also, ZarrV3ArrayNode.fill_value is restricted to number | string | boolean, but real Zarr stores may use null or more complex types. If you ever surface fill_value beyond debugging, you might want a looser type (e.g. unknown or number | string | boolean | null) to better reflect what can be in the metadata.

packages/zarrextra/src/index.ts (2)

15-101: Use per-level paths for attribute lookup when building the tree

In parseStoreContents, you currently call:

const attrs = getZattrs(normalizedPath as zarr.AbsolutePath, store);

inside the loop over pathParts. This means that when you synthesize intermediate group nodes for paths that don’t have their own metadata entries, they’ll look up attributes using the full leaf path and can accidentally inherit the leaf array’s attributes.

You can make attribute resolution more robust for both groups and arrays by computing the path prefix for the node you’re creating:

-    for (const [i, part] of pathParts.entries()) {
+    for (const [i, part] of pathParts.entries()) {
       if (!(part in currentNode)) {
         const isLeaf = i === pathParts.length - 1;
         const isArray = isLeaf && info.isArray;
-        
-        // Get attributes for this path (normalizedPath already has leading /)
-        const attrs = getZattrs(normalizedPath as zarr.AbsolutePath, store);
+        // Get attributes for this node using its own path prefix
+        const currentPath = `/${pathParts.slice(0, i + 1).join('/')}`;
+        const attrs = getZattrs(currentPath as zarr.AbsolutePath, store);
@@
           currentNode[part] = {
             [ATTRS_KEY]: attrs,
             [ZARRAY_KEY]: zarray,
             get: () => zarr.open(root.resolve(normalizedPath), { kind: 'array' })
           };

This keeps array lookups unchanged while ensuring group nodes only ever consult their own metadata paths.

---

134-224: Preserve v2 dtype when normalizing to v3 data_type

In normalizeV2ToV3Metadata, array nodes default data_type to 'float64' unless zarray.data_type exists:

data_type: (zarray.data_type as string) || 'float64',

For v2 .zarray metadata, the field is typically dtype, not data_type, so you’ll silently treat all such arrays as float64 in your normalized metadata even when the underlying dtype is different. If any downstream logic introspects data_type, it will see incorrect values.

You can cheaply preserve the v2 information by falling back to dtype:

-    if (group.zarray) {
-      // Array node - use zarray metadata as the base
-      const zarray = group.zarray as Record<string, unknown>;
-      metadata[path] = {
-        shape: (zarray.shape as number[]) || [],
-        data_type: (zarray.data_type as string) || 'float64',
+    if (group.zarray) {
+      // Array node - use zarray metadata as the base
+      const zarray = group.zarray as Record<string, unknown>;
+      const dtype = (zarray.data_type ?? zarray.dtype) as string | undefined;
+      metadata[path] = {
+        shape: (zarray.shape as number[]) || [],
+        data_type: dtype || 'float64',
@@
-        zarr_format: (zarray.zarr_format as number) || 3,
+        zarr_format: (zarray.zarr_format as number) || 3,

This keeps the internal v3-like shape while more accurately reflecting v2 array metadata.

.vscode/settings.json (1)

1-14: Consider whether to commit IDE settings.

Committing .vscode/settings.json directly can cause conflicts when team members have different setups (e.g., different Python versions or custom extensions). A common practice is to either:

1. Add .vscode/settings.json to .gitignore and provide a .vscode/settings.json.sample as a template
2. Keep only team-agreed settings and document others in README

This is minor since the settings are Python-related and align with the project's testing infrastructure.

.github/workflows/test.yml (2)

31-33: Consider pinning uv version for reproducibility.

Using version: "latest" may cause unexpected CI failures when a new uv version introduces breaking changes. Consider pinning to a specific version.

       - uses: astral-sh/setup-uv@v4
         with:
-          version: "latest"
+          version: "0.5.10"  # or current stable version

---

55-74: Add failure handling when server doesn't start.

If the server fails to start within 30 seconds, the loop exits silently and integration tests run anyway (likely failing with confusing errors). Consider adding explicit failure detection.

The shellcheck warning about unused i is a false positive for this retry-loop pattern.

           # Wait for server to be ready (up to ~30s)
+          SERVER_READY=false
           for i in {1..30}; do
             if curl -sSf http://localhost:8080/ >/dev/null; then
               echo "Test server is up"
+              SERVER_READY=true
               break
             fi
             echo "Waiting for test server on http://localhost:8080/ ..."
             sleep 1
           done

+          if [ "$SERVER_READY" != "true" ]; then
+            echo "ERROR: Test server failed to start within 30 seconds"
+            kill "$SERVER_PID" 2>/dev/null || true
+            exit 1
+          fi
+
           # Run integration tests (will hit http://localhost:8080/…)
           pnpm test:integration

scripts/cors-proxy.js (1)

40-119: Consider adding request timeout.

The fetch call has no timeout, which could cause the proxy to hang indefinitely if the target server doesn't respond. For a development tool, this may be acceptable, but consider adding a timeout for better developer experience.

     // Make the request
-    const response = await fetch(targetUrl, fetchOptions);
+    const controller = new AbortController();
+    const timeoutId = setTimeout(() => controller.abort(), 30000); // 30s timeout
+    
+    try {
+      const response = await fetch(targetUrl, { ...fetchOptions, signal: controller.signal });
+      clearTimeout(timeoutId);

scripts/test-server.js (2)

78-143: Consider adding OPTIONS preflight handler for CORS.

The server adds CORS headers to GET responses but doesn't handle OPTIONS preflight requests explicitly. While browsers may not send preflight for simple GET requests, adding OPTIONS support would ensure consistent behavior with the CORS proxy.

 async function handleRequest(req, res) {
   try {
+    // Handle OPTIONS preflight
+    if (req.method === 'OPTIONS') {
+      res.writeHead(200, {
+        'Access-Control-Allow-Origin': '*',
+        'Access-Control-Allow-Methods': 'GET, HEAD, OPTIONS',
+        'Access-Control-Allow-Headers': 'Range',
+      });
+      res.end();
+      return;
+    }
+
     // Remove query string and normalize path

---

91-99: Path traversal check has potential edge case with symlinks.

The check !fullPath.startsWith(fixturesDir) validates the resolved path, which is good. However, if fixturesDir contains symlinks pointing outside the fixtures directory, they could bypass this check. For a test server this is low risk, but consider using realpath for stricter validation if needed.

tests/unit/schemas.test.ts (1)

197-302: Consider adding negative test cases for invalid data.

The tests validate happy paths well, but could benefit from additional negative test cases, such as:

• Invalid encoding-type values
• Missing required fields in tableAttrsSchema
• Malformed coordinate systems

Example addition for tableAttrsSchema:

it('should reject missing required fields', () => {
  const attrs = {
    instance_key: 'cell_id',
    // missing region, region_key, spatialdata-encoding-type
  };

  expect(() => tableAttrsSchema.parse(attrs)).toThrow();
});

README.md (1)

184-186: Add language specifier to fenced code block.

The fenced code block is missing a language specifier. Based on static analysis hint (MD040).

-```
+```text
 http://localhost:8081/https://example.com/data.zarr/.zattrs

</blockquote></details>
<details>
<summary>package.json (1)</summary><blockquote>

`14-26`: **Well-organized test scripts with good naming conventions.**

The scripts provide a comprehensive testing workflow. Minor consideration:



The `validate:all` script uses `bash scripts/validate-all.sh` which may not work on Windows without WSL or Git Bash. Consider using a cross-platform alternative or documenting this limitation.

```diff
-    "validate:all": "bash scripts/validate-all.sh",
+    "validate:all": "node scripts/validate-all.js",

Alternatively, add a note in the README that this script requires a bash-compatible shell.

tests/integration/fixtures.test.ts (2)

16-35: Consider escaping version parameter in shell command.

While version comes from a controlled array, the use of string interpolation in execSync could be a concern if the test matrix is ever extended with external input.

 function ensureFixtures(version: string): string {
   const fixturePath = join(projectRoot, 'test-fixtures', `v${version}`, 'blobs.zarr');
   
   if (!existsSync(fixturePath)) {
     console.log(`Fixtures not found for version ${version}, generating...`);
     try {
-      execSync(`uv run python/scripts/generate_fixtures.py --version ${version}`, {
+      execSync(`uv run python/scripts/generate_fixtures.py --version "${version}"`, {
         cwd: projectRoot,
         stdio: 'inherit',
       });

Or use spawnSync with an arguments array to avoid shell interpretation entirely.

---

49-132: Tests silently pass when server is not running.

The tests return early with a console.warn when the server isn't running, which means they'll appear to pass in CI even when not actually exercised. Consider using it.skip() or throwing a skip exception to make this more visible.

     } catch (error) {
       if (error instanceof Error && error.message.includes('fetch')) {
-        console.warn(
-          'Skipping integration test - test server not running. ' +
-          'Start it with: pnpm test:server'
-        );
-        return;
+        // Use Vitest's skip API to make skipped tests visible
+        throw new Error(
+          'Test skipped: test server not running. Start it with: pnpm test:server'
+        );
       }
       throw error;
     }

Alternatively, use a beforeAll hook to check server availability and skip the entire describe block if unavailable.

packages/core/src/models/index.ts (1)

480-504: Error handling in loadElements prevents cascading failures.

The try-catch around createElement (lines 495-502) is a reasonable trade-off to prevent a single malformed element from breaking the entire load. The comment appropriately acknowledges the potential confusion of returning {} vs undefined.

Consider tracking failed elements and exposing them for debugging:

const result: Record<string, ElementInstanceMap[T]> = {};
const errors: Record<string, Error> = {};
for (const key of keys) {
  try {
    result[key] = createElement(name, sdata, key);
  } catch (error) {
    errors[key] = error as Error;
    console.error(`Failed to load ${name}/${key}:`, error);
  }
}
// Optionally return { elements: result, errors } or attach to metadata
return Object.keys(result).length > 0 ? result : undefined;

This would give consumers visibility into partial load failures.

scripts/validate-all.sh (1)

1-14: Script setup looks good, but consider adding dependency checks.

The script uses external tools (uv, pnpm, jq, node) without verifying they're available. Consider adding dependency checks at the start to provide clearer error messages.

 set -e  # Exit on error
 
+# Check required dependencies
+for cmd in uv pnpm jq node; do
+  if ! command -v "$cmd" &> /dev/null; then
+    echo -e "${RED}Error: Required command '$cmd' not found${NC}"
+    exit 1
+  fi
+done
+
 # Colors for output

python/v0.6.1/generate_fixtures.py (3)

27-29: Remove extraneous f-string prefix.

Line 29 has an f-string without any placeholders. As flagged by Ruff (F541).

     if actual_version != version:
         print(f"⚠️  Warning: Expected version {version} but got {actual_version}")
-        print(f"   This may indicate the wrong environment is active.")
+        print("   This may indicate the wrong environment is active.")

---

42-46: Move import to top of file.

The shutil import inside the function is unconventional. Consider moving it to the top-level imports for better readability and consistency with Python conventions.

 import sys
 from pathlib import Path
+import shutil
 
 # Add scripts directory to path if we need shared utilities

Then remove line 43:

-    import shutil

---

85-90: Rename unused loop variable.

The variable elem is not used within the loop body. As flagged by Ruff (B007).

             elif isinstance(elements, list):
                 print(f"  - {element_type}: {len(elements)} element(s)")
-                for i, elem in enumerate(elements):
+                for i, _elem in enumerate(elements):
                     print(f"    * element_{i}")

python/v0.5.0/generate_fixtures.py (6)

27-29: Remove extraneous f-string prefix.

Line 29 has an f-string without any placeholders. As flagged by Ruff (F541).

     if actual_version != version:
         print(f"⚠️  Warning: Expected version {version} but got {actual_version}")
-        print(f"   This may indicate the wrong environment is active.")
+        print("   This may indicate the wrong environment is active.")

---

50-54: Avoid bare except clause.

The bare except: will catch all exceptions including KeyboardInterrupt and SystemExit, which is generally undesirable. As flagged by Ruff (E722).

             try:
                 delattr(sdata, "points")
-            except:
+            except Exception:
                 # If that fails, set to empty dict
                 sdata.points = {}

---

72-79: Avoid bare except with pass and consider logging.

The try-except-pass pattern silently swallows errors. While this is defensive cleanup code, logging the exception would help with debugging. As flagged by Ruff (E722, S110).

     # Double-check it's gone
     if os.path.exists(store_path):
         print(f"Warning: Store path still exists after removal attempt: {store_path}")
         # Force remove
         try:
             shutil.rmtree(store_path, ignore_errors=True)
-        except:
-            pass
+        except Exception as e:
+            print(f"Warning: Force removal also failed: {e}")

---

93-102: Consider catching more specific exceptions for cleanup.

The broad Exception catch and subsequent bare except: make debugging harder. Consider at minimum logging the cleanup failure.

     except Exception as e:
         # Clean up any partially written store
         if os.path.exists(store_path):
             try:
                 if os.path.isdir(store_path):
                     shutil.rmtree(store_path)
                 else:
                     os.remove(store_path)
-            except:
-                pass
+            except OSError as cleanup_error:
+                print(f"Warning: Cleanup failed: {cleanup_error}")

---

131-134: Rename unused loop variable.

The variable elem is not used within the loop body. As flagged by Ruff (B007).

             elif isinstance(elements, list):
                 print(f"  - {element_type}: {len(elements)} element(s)")
-                for i, elem in enumerate(elements):
+                for i, _elem in enumerate(elements):
                     print(f"    * element_{i}")

---

20-148: Significant code duplication with v0.6.1 generator.

The generate_fixtures function shares substantial logic with python/v0.6.1/generate_fixtures.py. The main differences are the version string and the points workaround. Consider extracting shared utilities to reduce maintenance burden.

A shared module in python/scripts/ could contain:

• The element enumeration/printing logic (lines 121-144)
• The coordinate systems printing logic (lines 138-144)
• The common write fallback pattern

This would make version-specific scripts smaller and easier to maintain.

scripts/validate-datasets-js.js (1)

223-262: Consider using a CLI parsing library.

The manual argument parsing works but is verbose and error-prone. For a script of this complexity, consider using a library like commander or yargs, or at minimum, add validation for unknown arguments.

Currently unknown arguments are silently ignored. Consider:

} else if (!arg.startsWith('--')) {
  // Positional arguments - could be intentional or typo
} else {
  console.error(`Unknown option: ${arg}`);
  process.exit(1);
}

python/scripts/validate_datasets.py (5)

172-181: Remove unused exception variable.

The exception variable e is captured but never used. As per static analysis (Ruff F841).

-        except (json.JSONDecodeError, IndexError) as e:
+        except (json.JSONDecodeError, IndexError):

---

257-281: Remove unnecessary f-string prefixes.

Several strings have f prefix but contain no placeholders. As per static analysis (Ruff F541).

             if result.success:
-                lines.append(f"**Status:** ✅ Success")
+                lines.append("**Status:** ✅ Success")
                 lines.append("")

             else:
-                lines.append(f"**Status:** ❌ Failed")
+                lines.append("**Status:** ❌ Failed")
                 lines.append("")
                 lines.append(f"**Error Type:** `{result.error_type}`")
                 lines.append("")
-                lines.append(f"**Error Message:**")
+                lines.append("**Error Message:**")

---

219-225: Consider extracting version constants to avoid duplication.

The version strings "0.5.0" and "0.6.1" are hardcoded in multiple places (here, line 249, line 342, line 399). Extracting to a module-level constant would improve maintainability when adding new versions.

# Near top of file
SUPPORTED_VERSIONS = ["0.5.0", "0.6.1"]

---

401-413: Consider verifying environment directory exists before sync.

If the environment directory python/v{version} doesn't exist, uv sync will fail with a potentially confusing error. Adding a pre-check would provide clearer feedback.

     # Ensure environments are set up
     for version in versions:
         env_dir = project_root / "python" / f"v{version}"
+        if not env_dir.exists():
+            print(f"Error: Environment directory not found: {env_dir}", file=sys.stderr)
+            print("Please ensure the version-specific directories are set up.", file=sys.stderr)
+            sys.exit(1)
         print(f"Setting up environment for spatialdata {version}...", file=sys.stderr)

---

474-477: Unreachable else branch.

The argparse choices constraint ensures output_format is always one of markdown, csv, or json. The else branch is unreachable dead code.

     elif args.output_format == "json":
         output = json.dumps([r.to_dict() for r in results], indent=2)
-    else:
-        output = ""

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

scripts/validate-datasets-js.js (2)

60-114: Normalize thrown errors and stabilize the result schema for easier downstream consumption.

To make JSON/CSV consumers simpler and avoid surprises if a non-Error value is ever thrown, you can normalize error and always initialize stackTrace on the result:

 async function validateDataset(dataset, useProxy = false) {
   const result = {
     datasetName: dataset.name,
     datasetUrl: dataset.url,
     implementation: 'JavaScript (@spatialdata/core)',
     success: false,
     errorType: null,
     errorMessage: null,
     elements: null,
-    coordinateSystems: null,
+    coordinateSystems: null,
+    stackTrace: null,
   };

   try {
@@
-  } catch (error) {
-    result.success = false;
-    result.errorType = error.constructor.name;
-    result.errorMessage = error.message;
-
-    // Include stack trace for debugging
-    if (error.stack) {
-      result.stackTrace = error.stack.split('\n').slice(0, 5).join('\n');
-    }
-  }
+  } catch (error) {
+    const err = error instanceof Error ? error : new Error(String(error));
+    result.success = false;
+    result.errorType = err.name;
+    result.errorMessage = err.message;
+
+    if (err.stack) {
+      result.stackTrace = err.stack.split('\n').slice(0, 5).join('\n');
+    }
+  }

This keeps the schema consistent and avoids undefined accesses if something non-standard is thrown.

---

169-215: Consider making coordinate system names more readable in markdown output.

If result.coordinateSystems contains objects rather than plain strings, join(', ') will render them as [object Object]. You could improve readability like:

-      if (result.coordinateSystems && result.coordinateSystems.length > 0) {
-        lines.push(`**Coordinate Systems:** ${result.coordinateSystems.join(', ')}`);
+      if (result.coordinateSystems && result.coordinateSystems.length > 0) {
+        const csLabels = result.coordinateSystems.map(cs =>
+          typeof cs === 'string'
+            ? cs
+            : (cs && cs.name) || JSON.stringify(cs)
+        );
+        lines.push(`**Coordinate Systems:** ${csLabels.join(', ')}`);

If coordinateSystems are already strings, this is a no-op; otherwise it produces more meaningful labels.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: xinaesthete
Repo: Taylor-CCB-Group/SpatialData.js PR: 15
File: scripts/validate-all.sh:52-66
Timestamp: 2025-12-15T18:47:01.953Z
Learning: In the SpatialData.js monorepo, the validation scripts (scripts/validate-all.sh and scripts/validate-datasets-js.js) only depend on spatialdata/core and its dependency spatialdata/zarrextra, not on the react or vis packages. Therefore, when optimizing builds, it is sufficient to check only packages/core/dist before deciding to skip a build. Ensure this dependency boundary is enforced in review checks and that any changes to react/vis do not alter the validation flow.

Learnt from: xinaesthete
Repo: Taylor-CCB-Group/SpatialData.js PR: 15
File: scripts/validate-datasets-js.js:14-55
Timestamp: 2025-12-15T18:52:01.166Z
Learning: In the SpatialData official documentation's "Spatial omics datasets" page, there is a discrepancy between the "Filename" column and the actual working remote URLs in the "work with data remotely" column. When working with spatialdata datasets from S3, use the URLs from the "work with data remotely" column, not the filenames. Examples of differences: visium_hd_3.0.0_io.zarr/ vs visium_hd_3.0.0_id.zarr/ (filename vs remote URL), mouse_liver.zarr vs MouseLiver.zarr/, spacem_helanih3t3.zarr vs spacem_hepanih3t3.zarr/.

scripts/validate-datasets-js.js (1)

10-10: Redundant dynamic import of fs module.

writeFileSync is already imported from node:fs at line 10, but there's a dynamic import of fs at line 322 to use readFileSync. Consider using a static import for readFileSync alongside writeFileSync for consistency.

♻️ Suggested refactor

-import { writeFileSync } from 'node:fs';
+import { writeFileSync, readFileSync } from 'node:fs';

Then at line 322-324:

-      const fs = await import('node:fs');
-      const data = fs.readFileSync(args.comparePython, 'utf-8');
+      const data = readFileSync(args.comparePython, 'utf-8');

Also applies to: 321-324

README.md (1)

193-198: Add language specifier to fenced code block.

The fenced code block at line 195 is missing a language specifier, which triggers a markdownlint warning. Since this is a URL example, use text or an empty specifier.

📝 Suggested fix

 **Usage:**
 
 Proxy a remote URL by encoding it directly in the path:
-```
+```text
 http://localhost:8081/https://example.com/data.zarr/.zattrs

</details>

</blockquote></details>
<details>
<summary>python/scripts/validate_datasets.py (2)</summary><blockquote>

`172-172`: **Remove unused variable `e`.**

The exception variable `e` is assigned but never used. Use `_` to indicate it's intentionally unused, or remove it entirely.

<details>
<summary>♻️ Suggested fix</summary>

```diff
-        except (json.JSONDecodeError, IndexError) as e:
+        except (json.JSONDecodeError, IndexError):

---

259-280: Remove extraneous f prefix from strings without placeholders.

Lines 260, 276, and 280 use f-strings but contain no placeholders. Remove the f prefix.

♻️ Suggested fix

-                lines.append(f"**Status:** ✅ Success")
+                lines.append("**Status:** ✅ Success")

-                lines.append(f"**Status:** ❌ Failed")
+                lines.append("**Status:** ❌ Failed")

-                lines.append(f"**Error Message:**")
+                lines.append("**Error Message:**")

python/v0.7.0/generate_fixtures.py (1)

81-84: Rename unused loop variable.

The loop variable elem is not used within the loop body. Rename it to _elem to indicate it's intentionally unused.

♻️ Suggested fix

             elif isinstance(elements, list):
                 print(f"  - {element_type}: {len(elements)} element(s)")
-                for i, elem in enumerate(elements):
+                for i, _elem in enumerate(elements):
                     print(f"    * element_{i}")

scripts/validate-all.sh (3)

37-50: Consider adding set -o pipefail for safer pipeline handling.

With set -e, if the uv run command fails but the output redirection succeeds, the script will still exit. However, if you ever change this to use pipelines (e.g., command | tee file), errors in the first command could be masked. Adding set -o pipefail near line 6 would make the script more robust.

💡 Suggested improvement

-set -e  # Exit on error
+set -eo pipefail  # Exit on error and fail on pipeline errors

---

96-99: Missing dependency check for jq.

The script assumes jq is available but doesn't verify this. If jq is missing, the script will fail with an unhelpful error during the summary phase after already completing the time-consuming validation steps.

🛡️ Proposed fix to add dependency check

Add this near the top of the script (after the color definitions):

# Check required dependencies
for cmd in jq uv node pnpm; do
  if ! command -v "$cmd" &> /dev/null; then
    echo -e "${RED}Error: Required command '$cmd' not found.${NC}"
    exit 1
  fi
done

---

112-116: Symlink creation may fail silently on some systems.

The ln -s command creates a relative symlink, which is good. However, on some systems or edge cases (e.g., if $OUTPUT_DIR doesn't exist or has permission issues), this could fail. Consider adding error handling.

💡 Minor improvement for robustness

 # Create a symlink to latest results
 rm -f "$OUTPUT_DIR/latest"
-ln -s "$TIMESTAMP" "$OUTPUT_DIR/latest"
-echo -e "${GREEN}✓ Symlink created: $OUTPUT_DIR/latest${NC}"
+if ln -s "$TIMESTAMP" "$OUTPUT_DIR/latest" 2>/dev/null; then
+  echo -e "${GREEN}✓ Symlink created: $OUTPUT_DIR/latest${NC}"
+else
+  echo -e "${YELLOW}⚠ Could not create symlink (may not be supported on this filesystem)${NC}"
+fi

python/v0.6.1/generate_fixtures.py (1)

42-46: Consider moving shutil import to module level.

The import shutil inside the function works but is unconventional. Module-level imports improve readability and make dependencies explicit at the top of the file.

💡 Suggested change

Move the import to the top of the file with other imports:

 import sys
 from pathlib import Path
+import shutil

Then remove line 43:

-    # Remove existing store if it exists (to allow regenerating fixtures)
-    import shutil
+    # Remove existing store if it exists (to allow regenerating fixtures)

packages/core/src/models/VTableSource.ts (1)

27-36: Add /* @vite-ignore */ directive for external URL dynamic import.

Vite requires the /* @vite-ignore */ comment to skip import analysis on external URLs; otherwise it will attempt to prebundle/resolve and may fail or generate warnings. The webpackIgnore: true comment alone is insufficient—it only affects webpack, not Vite. Add the Vite directive alongside the existing webpack comment:

Suggested fix

-    const module = await import(/* webpackIgnore: true */ 'https://cdn.vitessce.io/parquet-wasm@2c23652/esm/parquet_wasm.js');
+    const module = await import(
+      /* `@vite-ignore` */ /* webpackIgnore: true */
+      'https://cdn.vitessce.io/parquet-wasm@2c23652/esm/parquet_wasm.js'
+    );

README.md (1)

196-199: Add language identifier to fenced code block.

The code block at line 197 is missing a language specifier. Since this is showing a URL format/pattern, use text or plaintext to satisfy markdown linting.

📝 Suggested fix

 **Usage:**
 
 Proxy a remote URL using query parameter:
-```
+```text
 http://localhost:8081/?url=https://example.com/data.zarr/.zattrs

</details>

</blockquote></details>
<details>
<summary>scripts/cors-proxy.js (3)</summary><blockquote>

`12-16`: **Variable shadowing: `resolve` from 'node:path' is shadowed in Promise callbacks.**

The `resolve` import from `node:path` is shadowed by the Promise `resolve` parameter in `createProxyServer` (line 175). While this works because the import isn't used in that scope, it can cause confusion. Consider renaming the import.



<details>
<summary>♻️ Suggested fix</summary>

```diff
 import { createServer } from 'node:http';
 import { URL, fileURLToPath } from 'node:url';
-import { resolve } from 'node:path';
+import { resolve as resolvePath } from 'node:path';

Then update line 211:

-const isMainModule = process.argv[1] && resolve(process.argv[1]) === __filename;
+const isMainModule = process.argv[1] && resolvePath(process.argv[1]) === __filename;

---

55-66: Consider including PATCH method in body forwarding.

The body forwarding logic handles POST and PUT, but PATCH requests also commonly include a body. This could cause issues if any proxied APIs use PATCH with payloads.

♻️ Suggested fix

     // Forward request body for POST/PUT
     let body = null;
-    if (req.method === 'POST' || req.method === 'PUT') {
+    if (req.method === 'POST' || req.method === 'PUT' || req.method === 'PATCH') {
       const chunks = [];
       for await (const chunk of req) {
         chunks.push(chunk);

---

191-206: Unbounded recursion risk in port retry logic.

The startProxyServer function recursively retries on the next port when EADDRINUSE occurs. In edge cases where many consecutive ports are occupied, this could lead to deep recursion or unexpected behavior. Consider adding a maximum retry limit.

🛡️ Suggested fix with retry limit

-export async function startProxyServer(port = 8081) {
+export async function startProxyServer(port = 8081, maxRetries = 10) {
+  if (maxRetries <= 0) {
+    throw new Error(`Could not find an available port after multiple retries (starting from ${port - 10})`);
+  }
   try {
     const server = await createProxyServer(port);
     // Get the actual port (in case it was changed due to port conflict)
     const actualPort = server.address()?.port || port;
     const baseUrl = `http://localhost:${actualPort}`;
     return { server, baseUrl };
   } catch (error) {
     // If port is in use, try next port
     if (error.code === 'EADDRINUSE') {
       console.error(`Port ${port} is in use, trying ${port + 1}...`);
-      return startProxyServer(port + 1);
+      return startProxyServer(port + 1, maxRetries - 1);
     }
     throw error;
   }
 }

scripts/validate-datasets-js.js (1)

298-307: CSV escaping is incomplete for some fields.

While elements, coordinateSystems, and errorMessage are now properly escaped, the datasetName, datasetUrl, and implementation fields are not. If any dataset name or URL contains double quotes (unlikely but possible), the CSV will be malformed.

♻️ Suggested fix for complete CSV escaping

   } else if (args.outputFormat === 'csv') {
     // Simple CSV generation
+    const csvEscape = (str) => str ? String(str).replace(/"/g, '""') : '';
     const lines = ['Dataset Name,Dataset URL,Implementation,Success,Error Type,Error Message,Elements,Coordinate Systems'];
     for (const r of results) {
       const elements = r.elements ? JSON.stringify(r.elements).replace(/"/g, '""') : '';
       const cs = r.coordinateSystems ? JSON.stringify(r.coordinateSystems).replace(/"/g, '""') : '';
-      const errorMsg = (r.errorMessage || '').replace(/"/g, '""');
-      lines.push(`"${r.datasetName}","${r.datasetUrl}","${r.implementation}",${r.success},"${r.errorType || ''}","${errorMsg}","${elements}","${cs}"`);
+      lines.push(`"${csvEscape(r.datasetName)}","${csvEscape(r.datasetUrl)}","${csvEscape(r.implementation)}",${r.success},"${csvEscape(r.errorType)}","${csvEscape(r.errorMessage)}","${elements}","${cs}"`);
     }
     output = lines.join('\n');

python/scripts/validate_datasets.py (1)

172-172: Remove unused variable e.

The exception variable is captured but never used.

♻️ Proposed fix

-        except (json.JSONDecodeError, IndexError) as e:
+        except (json.JSONDecodeError, IndexError):

python/scripts/validate_datasets.py (1)

219-230: Consider centralizing version constants.

The versions "0.5.0", "0.6.1", "0.7.0" are hardcoded here, in argparse.choices (line 344), and in the default versions list (line 401). If versions are added or removed, multiple locations need updating.

♻️ Suggested approach

Define a single constant near the top of the file:

SUPPORTED_VERSIONS = ["0.5.0", "0.6.1", "0.7.0"]

Then reference it in argparse, the default versions list, and dynamically generate table headers/columns.

packages/core/tests/schemas.spec.ts (4)

13-13: Clarify or address the TODO comment.

The comment "TODO: invert this type..." is ambiguous. Consider either:

• Adding more context about what needs to be inverted and why
• Creating a tracking issue if this is deferred work
• Removing if no longer applicable

---

117-119: Consider consolidating redundant parse calls.

The pattern of calling expect().not.toThrow() followed by another parse() is repeated throughout. You can simplify by parsing once and asserting on the result.

♻️ Suggested consolidation

-      expect(() => coordinateTransformationSchema.parse(transform)).not.toThrow();
-      const result = coordinateTransformationSchema.parse(transform);
-      expect(result[0].type).toBe('scale');
+      const result = coordinateTransformationSchema.parse(transform);
+      expect(result[0].type).toBe('scale');

If the parse succeeds, assertions on the result implicitly confirm it didn't throw. Reserve expect().not.toThrow() for tests where you only need to verify parsing succeeds without inspecting the result.

---

248-266: Consider adding a test for points attrs without transformations.

For consistency with shapesAttrsSchema tests (which include both with and without transformations), consider adding a similar negative case for pointsAttrsSchema.

Also, the comment on line 260 has the same issue as noted above - "(for raster elements)" is misleading for points attrs.

♻️ Suggested additional test

    it('should accept points attrs without transformations', () => {
      const attrs = {
        'encoding-type': 'ngff:points',
        axes: ['x', 'y'],
      };

      expect(() => pointsAttrsSchema.parse(attrs)).not.toThrow();
    });

---

293-322: Consolidate duplicate test cases.

The tests at lines 293-313 and 325-343 validate nearly identical scenarios (valid spatial data metadata with affine transformation). Similarly, lines 315-322 and 345-351 both test rejection of numeric version.

Consider consolidating these to reduce redundancy while maintaining coverage.

♻️ Suggested consolidation

Keep the more comprehensive tests and add comments noting they cover cases originally in index.test.ts:

-    // Tests from packages/core/src/index.test.ts
-    it('should validate spatial data metadata (from index.test.ts)', () => {
-      // coordinateSystems values are arrays of transformations
-      const validMetadata = {
-        version: '0.1.0',
-        coordinateSystems: {
-          global: [
-            {
-              type: 'affine',
-              affine: [
-                [1, 0, 0],
-                [0, 1, 0],
-              ],
-            },
-          ],
-        },
-      };
-
-      expect(() => spatialDataSchema.parse(validMetadata)).not.toThrow();
-    });
-
-    it('should reject invalid spatial data metadata (from index.test.ts)', () => {
-      const invalidMetadata = {
-        version: 123, // should be string
-      };
-
-      expect(() => spatialDataSchema.parse(invalidMetadata)).toThrow();
-    });

The existing tests at lines 293-322 already cover these cases adequately.

Also applies to: 325-351

packages/vis/tests/index.spec.tsx (1)

1-42: Test file structure looks good for API surface validation.

The tests appropriately verify that the public exports from @spatialdata/vis are defined and have the expected types. A few minor observations:

1. The file extension .tsx could be .ts since there's no JSX content.
2. The test at lines 30-41 partially duplicates checks from lines 11-17 (both verify the same component names).

Regarding the pipeline failure about @spatialdata/react: this appears to be a build/dependency resolution issue in the react package itself (possibly missing or incorrect main/module/exports in its package.json), not an issue with this test file.

packages/zarrextra/tests/zarrSchema.spec.ts (1)

130-226: Consider adding a test for v3 dimension_names length mismatch.

The v2 schema tests include a case for dimension_names length mismatch (lines 118-127), but the v3 schema tests don't have an equivalent. Based on the relevant code snippet showing that v3ZarraySchema has the same refinement rule for dimension_names, this edge case should also be tested for v3.

📝 Suggested test to add

it('should reject v3 zarray with dimension_names length mismatch', () => {
  const invalidZarray = {
    shape: [100, 200],
    data_type: 'float64',
    chunk_grid: {
      name: 'regular',
      configuration: { chunk_shape: [10, 20] },
    },
    dimension_names: ['y'], // Wrong length
  };

  expect(() => v3ZarraySchema.parse(invalidZarray)).toThrow();
});

python/scripts/validate_datasets.py (2)

219-220: Consider extracting the version list as a constant.

The version list ["0.5.0", "0.6.1", "0.7.0"] appears hardcoded in multiple places (table header and detailed results iteration). This creates maintenance overhead when adding new versions.

♻️ Optional refactor to centralize versions

+SUPPORTED_VERSIONS = ["0.5.0", "0.6.1", "0.7.0"]
+
 # In generate_markdown_table:
-    lines.append("| Dataset | v0.5.0 | v0.6.1 | v0.7.0 | URL |")
-    lines.append("|---------|--------|--------|--------|-----|")
+    version_headers = " | ".join(f"v{v}" for v in SUPPORTED_VERSIONS)
+    lines.append(f"| Dataset | {version_headers} | URL |")
+    lines.append("|---------|" + "--------|" * len(SUPPORTED_VERSIONS) + "-----|")

Also applies to: 251-251

---

341-347: Version list is duplicated across the codebase.

The version list appears in argparse.choices (line 344), main() default (line 401), and generate_markdown_table(). Centralizing this as a module-level constant would improve maintainability.

Also applies to: 400-401